Cumulus 200 #223
Cumulus 200 #223
Conversation
|
|
||
| program | ||
| .usage('TYPE COMMAND [options]') | ||
| .option('-s, --stack-name <stackName>', 'AWS Cloud Formation stack name', null) |
There was a problem hiding this comment.
Since this command will fail if any of these options are not set, that should be detected and handled. Right now there is nothing checking to see if these options (all of which are necessary) are set.
packages/integration-tests/index.js
Outdated
| * @param {string} workflowName - workflow name | ||
| * @returns {string} - workflow arn | ||
| */ | ||
| async function getWorkflowArn(stackName, bucketName, workflowName) { |
There was a problem hiding this comment.
This is not an async function. await is never used.
packages/integration-tests/index.js
Outdated
| * @param {string} stackName - Cloud formation stack name | ||
| * @param {string} bucketName - S3 internal bucket name | ||
| * @param {string} workflowName - workflow name | ||
| * @returns {string} - workflow arn |
There was a problem hiding this comment.
This returns a promise. @returns {Promise.<string>} - workflow arn
packages/integration-tests/index.js
Outdated
| * @param {string} workflowName - workflow name | ||
| * @returns {Object} template as a JSON object | ||
| */ | ||
| async function getWorkflowTemplate(stackName, bucketName, workflowName) { |
There was a problem hiding this comment.
This is not an async function, await is never used.
|
Should there be tests for this code? |
packages/integration-tests/index.js
Outdated
|
|
||
| // Give this execution a unique name | ||
| parsedInput.cumulus_meta.execution_name = uuidv4(); | ||
| parsedInput.cumulus_meta.workflow_start_time = null; |
There was a problem hiding this comment.
Why is this being set to null?
There was a problem hiding this comment.
Idk because I am a dum dum.
packages/integration-tests/index.js
Outdated
| statusCheckCount++; | ||
| } | ||
|
|
||
| if (executionStatus === 'RUNNING' && statusCheckCount === executionStatusNumRetries) { |
There was a problem hiding this comment.
Just for safety, statusCheckCount >= executionStatusNumRetries
packages/integration-tests/index.js
Outdated
| } | ||
|
|
||
| if (executionStatus === 'RUNNING' && statusCheckCount === executionStatusNumRetries) { | ||
| console.log(`Execution status check timed out, exceeded ${executionStatusNumRetries} status checks.`); |
There was a problem hiding this comment.
Add // eslint-disable-line max-len at the end of this line so it doesn't complain that the line is too long. In the case of long strings, exceeding the max line length isn't a problem.
packages/integration-tests/index.js
Outdated
| * @param {string} bucketName - S3 internal bucket name | ||
| * @returns {Object} list as a JSON object | ||
| */ | ||
| async function getWorkflowList(stackName, bucketName) { |
There was a problem hiding this comment.
This is never used and should be removed.
| * @returns {boolean} - true if all params are not null | ||
| */ | ||
| function verifyWorkflowParameters(requiredParams) { | ||
| return requiredParams.map((param) => verifyRequiredParameter(param)).includes(false) === false; |
There was a problem hiding this comment.
Instead of requiredParams.map((param) => verifyRequiredParameter(param)), just requiredParams.map(verifyRequiredParameter).
| * @returns {boolean} - true if all params are not null | ||
| */ | ||
| function verifyWorkflowParameters(requiredParams) { | ||
| return requiredParams.map((param) => verifyRequiredParameter(param)).includes(false) === false; |
There was a problem hiding this comment.
It's a little easier to read if it's written as return !requiredParams.map(verifyRequiredParameter).includes(false);, but that could just be my opinion.
There was a problem hiding this comment.
I am fundamentally opposed to the !
Summary: Start integration test prototype
Addresses CUMULUS-200: Integration test prototype
Changes
Test Plan
Things that should succeed before merging.
Reviewers: @scisco @yjpa7145 @jennyhliu @sethvincent